Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make altitude component of GpsInfo optional #69

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

felixc
Copy link
Owner

@felixc felixc commented Jan 23, 2023

This matches the real-world behaviour of apps that only set lat/long when drawing points on a map.

What made this tricky is that up until gexiv2 0.12.2, this was not necessary. If altitude (or indeed latitude or longitude) were missing, the function would still return successfully overall as long as at least one piece of data could be found. However in 0.12.2 this changes so that all three components need to be present for gexiv2_metadat_get_gps_info to return TRUE.

I am not sure if this change was intentional, so I have filed a bug upstream with gexiv2, but in the meantime we still need a workaround. Bug report: https://gitlab.gnome.org/GNOME/gexiv2/-/issues/72

Debian Stable as of today ships with 0.12.1, so I couldn't reproduce or test this locally, but the CircleCI Mac OSX executor is installing a recent version of the library via homebrew, so if the tests pass there it should be good.

This patch works around the problem by making the altitude (and only altitude) component of GpsInfo Optional. This is slightly annoyingly asymmetrical, but makes sense in reality as there's no reason to have, say, a latitude and an altitude but not a longitude. The workaround relies on the implementation detail that gexiv2 upstream happens to check and set latitude and longitude first, and altitude last. That means that even if we get a FALSE return, the latitude and longitude pointers may nonetheless have been populated, if the problem was simply that altitude was unset.

For compatibility with various versions of gexiv2, we handle both cases: the newer behaviour that returns FALSE, and also the older behaviour that returns TRUE but has altitude set to 0.0.

Addresses bug report #42

References:

This matches the real-world behaviour of apps that only set lat/long when
drawing points on a map.

What made this tricky is that up until gexiv2 0.12.2, this was not necessary.
If altitude (or indeed latitude or longitude) were missing, the function would
still return successfully overall as long as at least one piece of data could
be found. However in 0.12.2 this changes so that *all three* components need
to be present for `gexiv2_metadat_get_gps_info` to return `TRUE`.

I am not sure if this change was intentional, so I have filed a bug upstream
with gexiv2, but in the meantime we still need a workaround. Bug report:
  https://gitlab.gnome.org/GNOME/gexiv2/-/issues/72

Debian Stable as of today ships with 0.12.1, so I couldn't reproduce or test
this locally, but the CircleCI Mac OSX executor is installing a recent version
of the library via homebrew, so if the tests pass there it should be good.

This patch works around the problem by making the altitude (and only altitude)
component of `GpsInfo` `Optional`. This is slightly annoyingly asymmetrical,
but makes sense in reality as there's no reason to have, say, a latitude and
an altitude but not a longitude. The workaround relies on the implementation
detail that gexiv2 upstream happens to check and set latitude and longitude
first, and altitude last. That means that even if we get a `FALSE` return,
the latitude and longitude pointers may nonetheless have been populated, if
the problem was simply that altitude was unset.

For compatibility with various versions of gexiv2, we handle both cases: the
newer behaviour that returns `FALSE`, and also the older behaviour that
returns `TRUE` but has altitude set to `0.0`.

Addresses bug report #42 - #42

References:
  - 0.12.1 version of the code: https://gitlab.gnome.org/GNOME/gexiv2/blob/gexiv2-0.12.1/gexiv2/gexiv2-metadata-gps.cpp#L186
  - 0.12.2 version: https://gitlab.gnome.org/GNOME/gexiv2/blob/gexiv2-0.12.2/gexiv2/gexiv2-metadata-gps.cpp#L246
  - Commit that made the change: https://gitlab.gnome.org/GNOME/gexiv2/-/commit/f2d96b4df221d922ededfbff9236636273a56029
@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Base: 68.31% // Head: 71.15% // Increases project coverage by +2.83% 🎉

Coverage data is based on head (e8ceff8) compared to base (18008e5).
Patch coverage: 97.05% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #69      +/-   ##
==========================================
+ Coverage   68.31%   71.15%   +2.83%     
==========================================
  Files           1        1              
  Lines         950      981      +31     
==========================================
+ Hits          649      698      +49     
+ Misses        301      283      -18     
Impacted Files Coverage Δ
src/lib.rs 71.15% <97.05%> (+2.83%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant